fix: apply URDF material to child meshes when loadMeshCb returns a Group#333
fix: apply URDF material to child meshes when loadMeshCb returns a Group#333ferrolho wants to merge 5 commits into
Conversation
|
Thanks! Do you mind confirming what the behavior of the "material" tag is when loading URDF models across ROS tools? Both a Collada or OBJ, for example, can both have pre-existing materials that may have colors or textures assigned to them. Does this completely overwrite that? Or are the new colors multiplied in? One thing I've noticed (and have been frustrated by) about the ROS ecosystem is that behavior is often under-specified or modeled as "spec-by-implementation" so I'd just like to make sure this is aligned. |
|
In RViz, the URDF The URDF spec is under-specified and doesn't say which takes precedence. But the de facto standard (RViz) treats URDF color as a fallback for meshes that lack their own materials (e.g. STL, OBJ without MTL). I've updated the PR to match this behavior. Instead of blindly traversing all children, the new approach only applies the URDF material to child meshes whose material has no name (i.e. a loader default): if (obj instanceof THREE.Mesh) {
obj.material = material;
} else {
obj.traverse(child => {
if (child instanceof THREE.Mesh && !child.material.name) {
child.material = material;
}
});
}This preserves embedded Collada/GLTF materials while still applying URDF colors to meshes loaded without their own materials (the original bug this PR addresses). I've added a second test to verify embedded materials are preserved. |
|
Here's a before/after with the KUKA iiwa14, which uses OBJ meshes with URDF-defined material colors (Grey, Orange): Before —
After — URDF materials are applied to unnamed child meshes:
|
| @@ -590,6 +590,18 @@ class URDFLoader { | |||
There was a problem hiding this comment.
Thanks for the explanation. In this case lets remove this obj instanceof THREE.Mesh case and just always iterate over every mesh. I don't think there's a reason to alway replace the material if just a single object is returned like this.
There was a problem hiding this comment.
Removed the if/else branch. Now it always traverses the object tree. Since traverse visits the root too, a single Mesh is still handled correctly.
|
|
||
| obj.traverse(child => { | ||
|
|
||
| if (child instanceof THREE.Mesh && !child.material.name) { |
There was a problem hiding this comment.
I'm a bit concerned about this condition. I don't think there's any guarantee that a material is named when being loaded if it's not a "default" one. Eg a GLTF model may define materials with no name. I'm wondering if there's a better method for detecting this?
In practice what are the explicit cases where a mesh should have the materials replaced? Only if a format is loaded with NO materials? Like an STL or OBJ with no MTL file?
There was a problem hiding this comment.
Simplified to always apply the URDF material to all child meshes, which is consistent with how single-Mesh returns were already handled (unconditional overwrite). Users who need to preserve embedded materials can do so in their loadMeshCb.
There was a problem hiding this comment.
Sorry I didn't intend to request the change be reverted. My issue is that neither the previous material.name === '' approach nor this current one match the RViz behavior, which is why I asked about which mesh formats this applies to in practice.
File formats like glTF or COLLADA will never actually have a "default" or "unspecified" material assigned because they always specify materials in the file. It seems like this embedded URDF material is only really applicable to file formats like STL or OBJ (when no MTL is used), is that right?
Users who need to preserve embedded materials can do so in their loadMeshCb.
This isn't the case. The current implementation does not actually allow for this because it always overwrites the embedded materials with the URDF-specified ones after load.
2 participants
Summary
When
loadMeshCbreturns aTHREE.Group(e.g. fromOBJLoader), the URDF-defined material is not applied because the current code only sets.materialon directTHREE.Meshinstances. Meshes loaded without their own materials (e.g. OBJ without MTL) then keep the loader-default white material.However, some loaders return Groups with embedded materials that should be preserved (e.g.
ColladaLoader,GLTFLoader). In RViz, the URDF<material>tag acts as a fallback — it only applies when the mesh has no material of its own.This PR updates the material assignment to traverse child meshes, but only overwrite materials that have no name (i.e. loader defaults):
Test plan